Lexicon: A enumeration <-> string converter. by SolidWallOfCode · Pull Request #4222 · apache/trafficserver

SolidWallOfCode · 2018-09-09T16:07:27Z

This is a class I have intended on build for years, it is based on a class of the same name and similar function I wrote for Network Geographics. It was very handy but it required using Boost.MultiIndex which clearly wasn't going to work in the TS codebase. But now I have the infrastructure pieces needed to make the implementation simple, along with some nice C++ eleventy features. I ended up writing this now because it will be very useful in the YAML conversion of WCCP.

Dependent on #4223.

Waiting on other infrastructure PRs.

SolidWallOfCode · 2018-09-10T02:42:58Z

Current issue is a bug in clang interacting badly with the GCC headers, making std::variant essentially unusable in that situation. I'll need to rethink - I would rather not write my own version of std::variant.

ywkaras · 2018-09-10T15:18:50Z

doc/developer-guide/internal-libraries/Lexicon.en.rst

+Description
+===========
+
+A :class:`Lexicon` is templated by the enumeration class. It contains a set of **definitions**, each


I think it's more clear to say "a class template Lexicon takes an enumeration type as its only parameter".

I changed it around to specify the type should be numeric, rather than an enumeration specifically.

ywkaras · 2018-09-10T15:20:50Z

doc/developer-guide/internal-libraries/Lexicon.en.rst

+name, the primary name or any secondary name will yield the same enumeration.
+
+Defaults can be set so that any name or enumeration that does not match a definition yields the
+default value. The default can be explicit or it can be a handler function. The handler functions as


as -> acts as

ywkaras · 2018-09-10T15:28:43Z

doc/developer-guide/internal-libraries/Lexicon.en.rst

+default value. The default can be explicit or it can be a handler function. The handler functions as
+an internal catch for undefined conversions and is generally used to log the failure while returning
+a default. It could be used to compute a default but for names, this is problematic due to memory
+ownership and thread safety issues.


Highly non-obvious.

Reworded a bit, and a foot note added.

ywkaras · 2018-09-10T15:32:34Z

doc/developer-guide/internal-libraries/Lexicon.en.rst

+Assume the enumeration is
+
+.. literalinclude:: ../../../lib/ts/unit-tests/test_Lexicon.cc
+   :lines: 30


If you're going to do this, I think you need a "WARNING: Do not even BREATH on this file!" comment in test_Lexicon.cc.

The code this depends on is marked off as being part of the documentation. I really want to be able to know the example code compiles and runs.

The problem is for maintainers of test_Lexicon.cc to know that they need to change the documentation as well if they add or remove lines.

Maybe you should put code like this in test_Lexicon.cc: https://godbolt.org/z/b1H8tA

ywkaras · 2018-09-10T15:46:04Z

doc/developer-guide/internal-libraries/intrusive-hash-map.en.rst

-      Insert the element :arg:`v` into the container.
+      Insert the element :arg:`v` into the container. If there are already elements with the same
+      key, :arg:`v` is inserted after these elements.
+


Should we call it IntrusiveHashMultimap for consistency with STL naming?

Different PR, but no. Functionality is important, naming less so.

ywkaras · 2018-09-10T16:53:25Z

lib/ts/HashFNV.h

 template <typename Transform>
 void
-ATSHash64FNV1a::update(const void *data, size_t len, Transform xfrm)
+ATSHash64FNV1a::update(const void *data, size_t len, Transform xf)


Why in this case are we using 'Transform xf' instead of 'const Transform &xf'?

Oversight, I think the const& is a better choice in case Transform has internal state.

ywkaras · 2018-09-10T20:16:17Z

lib/ts/Lexicon.h

+ */
+template <typename E> class Lexicon
+{
+  using self_type = Lexicon; ///< Self reference type.


Why use alias that's longer than what it is aliasing?

Consistency, clarity. The point of using self_type is to be clear in declaration that a method is returning an instance of itself.

ywkaras · 2018-09-10T20:42:30Z

lib/ts/Lexicon.h

+  /// Storage for names.
+  MemArena _arena{1024};
+  /// Access by name.
+  IntrusiveHashMap<typename Item::NameLinkage> _by_name;


I think it's been shown that, for small lookup tables, an array/vector with linear search is fastest. In any case, it's simpler and any difference in speed will be small for a small lookup table.

Yes, but I don't agree that in this case it would be simpler, in terms of the implementation, in Lexicon. The mapping also gives us nice locality of equivalent keys.

ywkaras · 2018-09-10T20:46:03Z

lib/ts/Lexicon.h

+  std::string_view localize(std::string_view name);
+
+  /// Storage for names.
+  MemArena _arena{1024};


Won't the string names probably be string literals? If so, it seems a shame to copy them rather than just use the passed string_view.

Usually, but that can't be guaranteed. Because it's a MemArena the amortized cost of the copy is small and I think that's worth paying to avoid any (likely complex) rules about input string lifetimes, or restricting Lexicon to only handling constant strings.

ywkaras · 2018-09-10T21:16:08Z

lib/ts/unit-tests/test_IntrusiveHashMap.cc

+    for (decltype(len) j = 0; j < len; ++j) {
+      s[j] = char_gen(randu);
+    }
+    s[len] = 0;


Why the nul termination?

Not sure, I suspect it's a left over from a prior iteration when it used a C-string style interface instead of string_view.

maskit · 2018-09-11T02:49:31Z

Just out of curiosity, is there any mechanism that ensure all enum items are in the lexicon? Let's say Example::Value_4 was defined but there's no entry for it in the lexicon. What will happen if I access exnames[Example::Value_4]?

maskit · 2018-09-11T02:59:42Z

Ah, I found set_default.

maskit · 2018-09-11T03:07:12Z

But I'm still curious how we can avoid the mismatch. If it was a switch-case, some compilers warn it if the switch-case doesn't have default.

trafficserver/proxy/http2/Http2DebugNames.cc

Lines 29 to 47 in 4405411

    
           Http2DebugNames::get_settings_param_name(uint16_t id) 
        
           { 
        
             switch (id) { 
        
             case HTTP2_SETTINGS_HEADER_TABLE_SIZE: 
        
               return "HEADER_TABLE_SIZE"; 
        
             case HTTP2_SETTINGS_ENABLE_PUSH: 
        
               return "ENABLE_PUSH"; 
        
             case HTTP2_SETTINGS_MAX_CONCURRENT_STREAMS: 
        
               return "MAX_CONCURRENT_STREAMS"; 
        
             case HTTP2_SETTINGS_INITIAL_WINDOW_SIZE: 
        
               return "INITIAL_WINDOW_SIZE"; 
        
             case HTTP2_SETTINGS_MAX_FRAME_SIZE: 
        
               return "MAX_FRAME_SIZE"; 
        
             case HTTP2_SETTINGS_MAX_HEADER_LIST_SIZE: 
        
               return "MAX_HEADER_LIST_SIZE"; 
        
             } 
        
             return "UNKNOWN"; 
        
           }

SolidWallOfCode · 2018-09-11T19:04:15Z

There isn't any way, at compile time, to verify all of the enumeration values are in the Lexicon. If that is really important then one could use set_default with a function to log when a missing value is hit. There is also the problem that in many cases we have an extra value that shouldn't be there (e.g. LAST_VALUE). There are also use cases where the type is an integral type and not actually an enumeration. Nothing in the Lexicon API requires an enumeration.

I could look at creating a constructor like

template < size_t N > Lexicon(const std::array<Definition, N> &defines);

and then the initialization could be

Lexicon<Ex> ExNames(std::array<Lexicon<Ex>::Definition, Ex::LAST_VALUE> { ...});

In this case, if there are not exactly Ex::LAST_VALUE initializers for the array, it is a compile error.

ywkaras · 2018-09-11T19:11:09Z

A possible alternative is a macro-based approach: https://wkaras.webs.com/itemlist/itemlist.html

maskit · 2018-09-12T01:11:05Z

The mismatch would not be a big issue. I just wanted to know if it's possible.

maskit

Although I don't understand the implementation details but I'm ok with this. Only concern I have is maintainability. The code is much longer and more complicated than the switch-case example I posted above. I understand this is more general and more powerful, but I would switch back to switch-case way if I got into a trouble with this instead of fixing it.

SolidWallOfCode · 2018-09-12T01:24:40Z

@maskit Yes, but one of the key things this does the switch does not is provide conversion the other way. This is really quite useful for my work on configuration parsing and YAML, especially when the enumeration set is dynamic. It's just easier to update the list, and I have found in the past the secondary names are a nice touch in that area. As noted, this is a reconstruction of a class I used in my pre-TS work and I have missed it all these years. It's a joy to finally be at a point I can bring it back.

SolidWallOfCode · 2018-09-12T02:38:03Z

I added and documented a mechanism to do the size verification at compile time. Not quite as clunky as directly using a std::array.

ywkaras · 2018-09-12T15:15:03Z

include/tscore/HashFNV.h

  or more contributor license agreements.  See the NOTICE file
  distributed with this work for additional information
-  regarding copyright ownership.  The ASF licenses this file
+  regarding copyright ownership.  The ASF licenses this fileN


Is fileN really right?

That's actually in a different PR this depends on. #4223.

SolidWallOfCode · 2018-10-31T17:56:20Z

Leaving this until more of the infrastructure is in place.

SolidWallOfCode added the Core label Sep 9, 2018

SolidWallOfCode added this to the 9.0.0 milestone Sep 9, 2018

SolidWallOfCode self-assigned this Sep 9, 2018

SolidWallOfCode requested a review from randall September 9, 2018 16:07

SolidWallOfCode force-pushed the lexicon branch 2 times, most recently from 57360fb to 48281f6 Compare September 10, 2018 02:12

ywkaras reviewed Sep 10, 2018

View reviewed changes

maskit reviewed Sep 12, 2018

View reviewed changes

SolidWallOfCode force-pushed the lexicon branch from ea7ff14 to 0eea3f4 Compare September 12, 2018 02:32

SolidWallOfCode force-pushed the lexicon branch from 0eea3f4 to 8f254e6 Compare September 12, 2018 02:58

ywkaras reviewed Sep 12, 2018

View reviewed changes

SolidWallOfCode added 2 commits September 15, 2018 11:58

Hash: Add hash_immediate to generic hash and FNV hashes.

3803eb2

Lexicon: A enumeration <-> string converter.

13f9185

SolidWallOfCode force-pushed the lexicon branch from 8f254e6 to 13f9185 Compare September 15, 2018 18:18

SolidWallOfCode added the WIP label Oct 12, 2018

SolidWallOfCode closed this Oct 31, 2018

maskit mentioned this pull request Nov 7, 2018

Include additional names of events for debug output. #4566

Merged

zwoop removed this from the 9.0.0 milestone Jan 7, 2019

SolidWallOfCode deleted the lexicon branch July 27, 2021 15:10

Conversation

SolidWallOfCode commented Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SolidWallOfCode commented Sep 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywkaras Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maskit commented Sep 11, 2018

Uh oh!

maskit commented Sep 11, 2018

Uh oh!

maskit commented Sep 11, 2018

Uh oh!

SolidWallOfCode commented Sep 11, 2018

Uh oh!

ywkaras commented Sep 11, 2018

Uh oh!

maskit commented Sep 12, 2018

Uh oh!

maskit left a comment

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode commented Sep 12, 2018

Uh oh!

SolidWallOfCode commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode commented Oct 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

SolidWallOfCode commented Sep 9, 2018 •

edited

Loading

ywkaras Sep 12, 2018 •

edited

Loading